Continuation of JWKS support from PR #71#85
Continuation of JWKS support from PR #71#85davidallendj wants to merge 6 commits intogo-chi:masterfrom
Conversation
|
@pkieltyka Can I get a review for this? |
|
|
||
| func (ja *JWTAuth) Encode(claims map[string]interface{}) (t jwt.Token, tokenString string, err error) { | ||
| if ja.keySet != nil { | ||
| return nil, "", fmt.Errorf("encode not supported") |
There was a problem hiding this comment.
I'm working on rebasing and adding more documentation. I don't understand why encode isn't supported in this case and I think we need a better error message for it. Can you explain what this is for?
There was a problem hiding this comment.
I think that's the wrong error message probably from some copy-pasta. I think the message should just reflect that the JWKS isn't set so nothing can be encoded here.
Edit: This may have been something added by the original author, so I'm not 100% sure of the intent here, so I'm speculating.
There was a problem hiding this comment.
Based on my understanding, I'm considering updating Encode like this:
// Encode generates a JWT token string with the provided claims.
// It returns the encoded token as a string, along with the token object and any error encountered.
// If the JWTAuth instance has a key set, encoding is not supported and an error is returned.
func (ja *JWTAuth) Encode(claims map[string]interface{}) (t jwt.Token, tokenString string, err error) {
if ja.keySet != nil {
return nil, "", fmt.Errorf("encoding is not supported with key set")
}
t = jwt.New()
for k, v := range claims {
if err := t.Set(k, v); err != nil {
return nil, "", err
}
}
payload, err := ja.sign(t)
if err != nil {
return nil, "", err
}
tokenString = string(payload)
return
}There was a problem hiding this comment.
Looks fine to me. I was reading the code wrong and was thinking the ja.keySet was an error. I'll have to go back and look at the entire PR to recall the context why this was done.
There was a problem hiding this comment.
I'm refactoring it a bit now that I understand it better.
This PR continues the work from PR #71 to add JWKS support, but removes the dynamic verifier as requested.